Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Begin porting the web POC to react native #10

Merged
merged 45 commits into from
Aug 10, 2020
Merged

Conversation

tgolen
Copy link
Contributor

@tgolen tgolen commented Aug 9, 2020

Lots of stuff in this!

  1. Displays personal details in the header
  2. Displays the list of reports in the sidebar (but the links don't do anything yet).
  3. Switches web to use a hash router which should help with the refreshing
  4. Adds direct binding the store to the state using an HOC

image

@tgolen tgolen requested a review from AndrewGable August 9, 2020 04:28
@tgolen tgolen self-assigned this Aug 9, 2020
Copy link
Contributor

@AndrewGable AndrewGable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web works great and the code is very clean! I left some comments from my testing on iOS. I am totally happy leaving the harder ones as TODOs in the code if we mark them as such and looking at them later.

src/store/actions/PersonalDetailsActions.js Show resolved Hide resolved
src/page/HomePage/HeaderView.js Outdated Show resolved Hide resolved
src/style/StyleSheet.js Outdated Show resolved Hide resolved
padding: 8,
},
mainContentWrapper: {
height: 'calc(100vh - 73px)',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mobile does not like this line, it chokes on it. Is there anyway to refactor it to not use calc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah phooey. I can keep trying, yeah.

src/Expensify.js Outdated
<Route path="/" component={HomePage} />
</Switch>
</Router>
<Beforeunload onBeforeunload={ActiveClientManager.removeClient}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mobile doesn't play well with this, it seems to use window quite a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, this was supposed to be compatible :(

Copy link
Contributor

@AndrewGable AndrewGable Aug 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not an expert on this event, but could we use AppState instead? It is supported by RN and RN4W out of the box: https://github.com/necolas/react-native-web#modules

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this and yes it does change to inactive when the tab is changed, then back to active when it comes back. This sounds like possibly we need to handle it using a native/web solution (like Router).

const myPersonalDetails = allPersonaDetails[currentLogin];
return Store.multiSet({
[STOREKEYS.PERSONAL_DETAILS]: allPersonaDetails,
[STOREKEYS.MY_PERSONAL_DETAILS]: myPersonalDetails,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you see this on web but iOS complains whenever something is set that is undefined (as this is on boot), not sure if or when we want to set a default value, but it could keep the logs from being too noisy if we added one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I've seen those errors, no. What is undefined when that happens. Is it myPersonalDetails or allPersonaDetails (which I just realized has a typo)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was myPersonalDetails that was undefined

src/page/HomePage/SidebarView.js Outdated Show resolved Hide resolved
src/lib/ActiveClientManager.js Outdated Show resolved Hide resolved
Comment on lines 53 to 56
<Text style={textActiveStyle}>{this.props.reportName}</Text>
{this.state.isUnread && (
<Text style={textActiveStyle}>- Unread</Text>
)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly clear why, but this needs to be wrapped in a view for iOS to work.

Suggested change
<Text style={textActiveStyle}>{this.props.reportName}</Text>
{this.state.isUnread && (
<Text style={textActiveStyle}>- Unread</Text>
)}
<View>
<Text style={textActiveStyle}>{this.props.reportName}</Text>
{this.state.isUnread && (
<Text style={textActiveStyle}>- Unread</Text>
)}
</View>

},
sidebarLinkActive: {
backgroundColor: '#007bff',
borderRadius: '.25rem',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iOS chokes on rem values too 😢

@tgolen
Copy link
Contributor Author

tgolen commented Aug 9, 2020 via email

@AndrewGable AndrewGable merged commit 9c479ff into master Aug 10, 2020
@AndrewGable AndrewGable deleted the tgolen-port-webpoc branch August 10, 2020 00:07
AndrewGable pushed a commit that referenced this pull request Mar 4, 2021
Julesssss pushed a commit that referenced this pull request Apr 7, 2021
@OSBotify
Copy link
Contributor

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Oct 3, 2023

🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.3.77-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Oct 5, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.77-7 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

MrMuzyk pushed a commit to MrMuzyk/App that referenced this pull request Jan 25, 2024
* fix: ubo list hover, incorp state hover

* feat: improved return type for getFieldRequiredErrors
hayata-suenaga pushed a commit that referenced this pull request Feb 1, 2024
…-settings

Improve workspace settings on Main Pane
francoisl pushed a commit that referenced this pull request Feb 13, 2024
Copy link

Deploying helpdot with  Cloudflare Pages  Cloudflare Pages

Latest commit: f5512a9
Status: ✅  Deploy successful!
Preview URL: https://d7223a32.helpdot.pages.dev

View logs

thienlnam pushed a commit that referenced this pull request Apr 16, 2024
…tion-report-view

Revert "Add One Transaction Report View"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants